NET-1721: Automatic ACL bootstrap with Vault secrets backend #1920

pglass · 2023-02-17T01:05:23Z

Changes proposed in this PR:

This updates server-acl-init to support automatic ACL bootstrapping when using the Vault secrets backend.

In order to accomplish this, the server-acl-init job runs the Vault agent as a sidecar (in addition to running as an init container). If the bootstrap token is not found in Vault, then server-acl-init will proceed with ACL bootstrapping and write the token back to Vault.

Because server-acl-init writes to Vault via the Vault agent, server-acl-init doesn't have to worry specifying a Vault token or TLS config that it would normally need to talk to the Vault servers.

This adds the Vault SDK to the control-plane binary, which is +1 MB to the consul-k8s-control-plane binary size (74MB to 75MB).

Related to #1176

How I've tested this PR:

Acceptance tests

How I expect reviewers to test this PR:

👀

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

cthain

This is great stuff Paul. I love the secrets backend abstraction! I left a few minor nits/comments/questions.

charts/consul/templates/server-acl-init-job.yaml

control-plane/subcommand/server-acl-init/command.go

cthain · 2023-02-17T17:37:40Z

control-plane/subcommand/server-acl-init/command.go

+			ctx:          c.ctx,
+			clientset:    c.clientset,
+			k8sNamespace: c.flagK8sNamespace,
+			// TODO: should these use the global.acls.bootstrapToken.{secretName,secretKey}?


👍 I think that is reasonable and intuitive since there's nothing Vault-specific about the bootstrap token fields. It feels to me like the behavior for Vault and K8s should be the same. Is there a good reason not to do this? Would it break any existing behavior/assumptions?

I think I can't change this (yet) because it may break existing deployments.

If ACLs are pre-bootstrapped, then I would specify the k8s secret containing the bootstrap token in the Helm values, and server-acl-init reads from the specified secret

If server-acl-init automatically bootstrapped ACLs, then it writes the token to the hard-coded secret (<prefix>-bootstrap-acl-token).

If we change the secret name, then server-acl-init would fail to find the bootstrap token in case 2. (I'm not clear how much of a problem this is if server-acl-init has already run once? But there are lots of upgrade scenarios to think through that it feels dangerous to do with my current limited understanding)

To make k8s and Vault secrets backends more consistent, I updated the secrets backends to use the given secret name, if provided, or else use a default secret name. This applies to both k8s and Vault.

I also removed the -bootstrap-token-file flag and the mounts/files that were used for the k8s and Vault secrets. Instead, the secret is read by server-acl-init directly from the secrets backend API (k8s or vault).

This should not be a breaking change because if a secret exists and contains the token, then that token is used (same as before). If the secret is empty, then we proceed with ACL bootstrapping and update the secret (previously, server-acl-init would error out).

Let me know what you think, and cc @thisisnotashwin for your thoughts on this

control-plane/subcommand/server-acl-init/command.go

control-plane/subcommand/server-acl-init/secrets_backend.go

jkirschner-hashicorp · 2023-02-17T19:11:31Z

Does this PR need to consider the case when the server agents are external (e.g., on VMs, in HCP, on a different k8s cluster)?

Would the user set manageSystemACLs to true, and then if they didn't specify a bootstrap token, the bootstrapping operation would fail (or not work as expected) because the Consul server agents have already had their ACL system bootstrapped?

Happy to follow up over Zoom, not sure I've articulated this well. I just want to make sure we've considered how this might work in the context where the Consul server agents aren't in the current k8s cluster.

pglass · 2023-02-17T19:43:14Z

@jkirschner-hashicorp

Does this PR need to consider the case when the server agents are external (e.g., on VMs, in HCP, on a different k8s cluster)?

This should work the same as before w.r.t whether servers are external or not. The ACL bootstrapping logic was already there. This just changes where the bootstrap token can be written (i.e. can also store it in Vault).

Would the user set manageSystemACLs to true, and then if they didn't specify a bootstrap token, the bootstrapping operation would fail (or not work as expected) because the Consul server agents have already had their ACL system bootstrapped?

Right. If no bootstrap token provided, it will attempt to ACL bootstrap. If it tries to bootstrap a cluster that is already ACL bootstrapped, then server-acl-init will fail. You'll have to go and update the Helm values to set the bootstrap token.

pglass · 2023-02-21T21:08:33Z

charts/consul/templates/server-acl-init-job.yaml

+        {{- if .Values.global.secretsBackend.vault.enabled }}
+        {{- if .Values.global.secretsBackend.vault.agentAnnotations }}
+        - name: VAULT_NAMESPACE
+          value: {{ get (tpl .Values.global.secretsBackend.vault.agentAnnotations . | fromYaml) "vault.hashicorp.com/namespace" }}


This ended up being semi-ugly. To specify a Vault namespace for the Vault agent, you add a vault.hashicorp.com/namespace annotation. With the Consul helm chart, this is specified in the "extra" global.secretsBackend.vault.agentAnnotations. So I had to parses the Vault namespace from those extra annotations.

When talking through the Vault agent, even though it uses the namespace for injected secrets/templates, the agent doesn't scope API requests to that namespace I guess. So this needs to be explicitly specified for the Vault client used by server-acl-init when reading/writing secrets in the Vault API.

thisisnotashwin

great work here bud!! sorry for the late review.

david-yu · 2023-02-28T18:42:01Z

Added backport/1.1.0.x to this PR. If you feel this should be back ported to other versions, please add labels and ensure the docs are updated for each branch. Thanks!

david-yu · 2023-03-04T04:56:11Z

Is this ready to merge?

With the Vault secrets backend, server-acl-init now: * Runs the Vault agent as a sidecar * Bootstraps ACLs if the Vault bootstrap token is empty or not found, and writes the bootstrap token back to Vault via the Vault agent This adds the Vault SDK to the control-plane binary. This added 1 MB to the binary size (74MB to 75MB)

Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>

* The Kubernetes backend will write the bootstrap token to the user-provided secret if that secret is empty. The Vault behavior is the same. * The Vault backend writes to a default secret name if the secretName and secretKey are not set in the helm chart values. * Pass the Vault namespace to server-acl-init server-acl-init reads the secret directly from k8s or Vault. * Remove -bootstrap-token-file flag from server-acl-init and remove the * Remove the volume/mount for that. And update all the tests for that. Remove the bootstrap token secret injection / template the Vault agent.

Support automatic ACL bootstrapping with the Vault secrets backend With the Vault secrets backend, server-acl-init now: * Runs the Vault agent as a sidecar * Bootstraps ACLs if the Vault bootstrap token is empty or not found, and writes the bootstrap token back to Vault via the Vault agent The Kubernetes backend will write the bootstrap token to the user-provided secret if that secret is empty. The Vault behavior is the same. The Vault backend writes to a default secret name if the secretName and secretKey are not set in the helm chart values. server-acl-init reads the secret directly from k8s or Vault. * Remove -bootstrap-token-file flag from server-acl-init and remove the * Remove the volume/mount for bootstrap token --------- Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>

pglass requested review from a team, skpratt, kisunji and cthain and removed request for a team and kisunji February 17, 2023 01:05

pglass mentioned this pull request Feb 17, 2023

Helm + VaultBackend + ACLs | Bootstarp ACLs and store the bootstrapToken and the replicationToken in Vault #1176

Closed

david-yu linked an issue Feb 17, 2023 that may be closed by this pull request

Helm + VaultBackend + ACLs | Bootstarp ACLs and store the bootstrapToken and the replicationToken in Vault #1176

Closed

cthain reviewed Feb 17, 2023

View reviewed changes

pglass requested a review from thisisnotashwin February 21, 2023 20:54

pglass commented Feb 21, 2023

View reviewed changes

thisisnotashwin approved these changes Feb 27, 2023

View reviewed changes

david-yu added the backport/1.1.x Backport to release/1.1.x branch label Feb 28, 2023

pglass force-pushed the pglass/NET-1721-vault-secrets-backend-boostrap branch from 61fe6f8 to 0c43247 Compare February 28, 2023 18:53

Paul Glass and others added 4 commits March 6, 2023 09:32

Apply suggestions from code review

a59a16a

Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>

Update changelog

583d545

pglass force-pushed the pglass/NET-1721-vault-secrets-backend-boostrap branch from 0c43247 to 583d545 Compare March 6, 2023 15:32

Paul Glass added 2 commits March 6, 2023 09:33

Fix changelog

2ae60da

Fix changelog again

8ebd3e7

pglass merged commit 7079fc8 into main Mar 6, 2023

pglass deleted the pglass/NET-1721-vault-secrets-backend-boostrap branch March 6, 2023 19:11

hc-github-team-consul-core mentioned this pull request Mar 6, 2023

Backport of NET-1721: Automatic ACL bootstrap with Vault secrets backend into release/1.1.x #1988

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NET-1721: Automatic ACL bootstrap with Vault secrets backend #1920

NET-1721: Automatic ACL bootstrap with Vault secrets backend #1920

pglass commented Feb 17, 2023 •

edited

Loading

cthain left a comment

cthain Feb 17, 2023

pglass Feb 17, 2023 •

edited

Loading

pglass Feb 21, 2023 •

edited

Loading

jkirschner-hashicorp commented Feb 17, 2023

pglass commented Feb 17, 2023 •

edited

Loading

pglass Feb 21, 2023 •

edited

Loading

thisisnotashwin left a comment

david-yu commented Feb 28, 2023

david-yu commented Mar 4, 2023

NET-1721: Automatic ACL bootstrap with Vault secrets backend #1920

NET-1721: Automatic ACL bootstrap with Vault secrets backend #1920

Conversation

pglass commented Feb 17, 2023 • edited Loading

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

cthain left a comment

Choose a reason for hiding this comment

cthain Feb 17, 2023

Choose a reason for hiding this comment

pglass Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

pglass Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

jkirschner-hashicorp commented Feb 17, 2023

pglass commented Feb 17, 2023 • edited Loading

pglass Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

thisisnotashwin left a comment

Choose a reason for hiding this comment

david-yu commented Feb 28, 2023

david-yu commented Mar 4, 2023

pglass commented Feb 17, 2023 •

edited

Loading

pglass Feb 17, 2023 •

edited

Loading

pglass Feb 21, 2023 •

edited

Loading

pglass commented Feb 17, 2023 •

edited

Loading

pglass Feb 21, 2023 •

edited

Loading